Repository navigation
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes add managed sandbox email sender provisioning, recipient checks, fixed-identity delivery, and sender controls across workflow email surfaces. They also add start metadata to workflow templates and introduce a feature-flagged first-run gallery and workflow customization flow. The first-run flow selects templates based on observed events, supports email edits and test sends, and creates workflows in draft or active status. Priority: ➖ Normal Merge Risk: 🔵 Low · up to The sandbox sender and first-run workflow paths are behind feature flags, and recipient checks fail closed. Two small follow-ups remain. One template describes a start condition that differs from its actual trigger. A misconfigured sandbox sender would fail its sends instead of being caught at startup. Neither issue blocks merging, but both are worth fixing. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Sender ownership and recipient restrictions are enforced on the server. However, activating the shared sender depends on separate abuse-containment protections and coordinated email-provider configuration. Delivery is disabled by default, which substantially limits immediate exposure. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
Full details: Description checkExplanation The description has the required sections and detailed test evidence, but it does not match the supplied change summary. It describes a five-file banner-only change, while the summary includes extensive first-run workflow and sandbox email sender functionality. Resolution Update the description to cover the full scope of the supplied changes, including the first-run workflow experience, sandbox sender provisioning and delivery, validation, and template start conditions. Correct the file and line counts, explain the user-visible changes and release status for the full PR, and revise any statements that say these features are outside the PR. Alternatively, split the changes so this PR contains only the banner work described. ✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/workflows/backend/templates/onboarding_started_but_not_completed_template.json-4-8 (1)
4-8: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
starts_on.detaildoes not match the trigger filter.
detailsays "on /onboarding". The trigger filters$current_url icontains "messaging/campaigns"on lines 39 and 133. The gallery tells users that the template starts on onboarding pageviews. The workflow actually fires only on campaign-page URLs. Align the trigger URL value with/onboarding, or changedetailto describe the real filter.
🧹 Nitpick comments (4)
nodejs/src/cdp/cdp-services.ts (1)
464-472: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFail fast when the sandbox sender is enabled but not fully configured.
WORKFLOWS_SANDBOX_SENDER_ENABLED=truecan be set whileSES_SANDBOX_FROM_ADDRESS,SES_SANDBOX_TENANT_NAME, orSES_SANDBOX_CONFIGURATION_SETstays at its''default. In that caseresolveFromSenderreturnsemail: ''andsesRouteForreturns an empty tenant and configuration set. Every sandbox send then reaches SES with"Name" <>and fails asemail_failed. Nothing reports the misconfiguration at startup. Fix: validate the three settings here, and either throw or forceenabledto false with an error log.♻️ Proposed guard
+ const sandboxConfigured = !!( + config.SES_SANDBOX_TENANT_NAME && + config.SES_SANDBOX_CONFIGURATION_SET && + config.SES_SANDBOX_FROM_ADDRESS + ) + if (config.WORKFLOWS_SANDBOX_SENDER_ENABLED && !sandboxConfigured) { + logger.error('📧', 'Sandbox sender enabled without tenant, configuration set, and from address; disabling it') + } new SandboxEmailSender( { - enabled: config.WORKFLOWS_SANDBOX_SENDER_ENABLED, + enabled: config.WORKFLOWS_SANDBOX_SENDER_ENABLED && sandboxConfigured,products/workflows/frontend/firstRun/firstRunSenderLogic.ts (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
IntegrationTypethrough the~/typesalias.
'../../../../frontend/src/types'climbs out of the product package to reach the same module that every other file imports as~/types. This adds a second import path for one symbol, and the path breaks if the folder moves. Useimport type { IntegrationType } from '~/types'.♻️ Proposed fix
-import type { IntegrationType } from '../../../../frontend/src/types' +import type { IntegrationType } from '~/types'As per coding guidelines: "Every symbol has exactly one home. No re-export shims creating a second import path".
Source: Coding guidelines
products/workflows/backend/services/sandbox_sender.py (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDefine the
workflows-sandbox-senderflag key once. Provisioning and save-time validation each hardcode the flag key. If one copy is renamed, the two checks read different flags.
products/workflows/backend/services/sandbox_sender.py#L14-L14: removeSANDBOX_SENDER_FLAGhere. Move it toposthog/models/integration/email.pynext toSANDBOX_EMAIL_PROVIDER, export it fromposthog/models/integration/__init__.py, and import it here.posthog/cdp/validation.py#L148-L155: replace the"workflows-sandbox-sender"literal with the shared constant.products/workflows/backend/presentation/views/hog_flow.py (1)
2270-2270: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueExclude sandbox integrations from the sharer query too.
Line 2270 removes sandbox integrations from this team's domain list only. The sharer query below (lines 2280-2284) has no matching exclusion. The sandbox domain is shared by every team, so it does not reach
domainstoday. If a team's own verified domain ever matches the sandbox domain, every team with a provisioned sandbox sender counts as a sharer. The domain then goes intowithheldfor most callers. Add the same.exclude(config__contains={"provider": SANDBOX_EMAIL_PROVIDER})to the sharer query so both queries treat sandbox senders the same way.Proposed fix
Integration.objects.filter(kind="email", config__verified=True, config__domain__in=domains) .exclude(team_id=team.id) + .exclude(config__contains={"provider": SANDBOX_EMAIL_PROVIDER}) .values_list("config__domain", "team_id")
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
b35b7f74-5c64-4071-8ec6-5f4545b0e37c
⛔ Files ignored due to path filters (3)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlproducts/integrations/frontend/generated/api.tsis excluded by!**/generated/**products/workflows/frontend/generated/api.schemas.tsis excluded by!**/generated/**
📒 Files selected for processing (104)
docs/internal/workflows-email-sending-tiers.mdfrontend/src/lib/components/CyclotronJob/CyclotronJobInputs.tsxfrontend/src/lib/constants.tsxfrontend/src/lib/integrations/IntegrationsList.tsxfrontend/src/lib/integrations/integrationsLogic.test.tsfrontend/src/lib/integrations/integrationsLogic.tsfrontend/src/lib/integrations/utils.tsfrontend/src/scenes/hog-functions/email-templater/EmailTemplater.test.tsxfrontend/src/scenes/hog-functions/email-templater/EmailTemplater.tsxfrontend/src/scenes/hog-functions/email-templater/NativeEmailIntegrationChoice.stories.tsxfrontend/src/scenes/hog-functions/email-templater/emailTemplaterLogic.test.tsfrontend/src/scenes/hog-functions/email-templater/emailTemplaterLogic.tsxfrontend/src/scenes/hog-functions/email-templater/selectSenders.test.tsfrontend/src/scenes/hog-functions/email-templater/selectSenders.tsnodejs/package.jsonnodejs/src/cdp/cdp-services.tsnodejs/src/cdp/config.tsnodejs/src/cdp/services/managers/organization-members.service.tsnodejs/src/cdp/services/messaging/email.service.test.tsnodejs/src/cdp/services/messaging/email.service.tsnodejs/src/cdp/services/messaging/sandbox-email-sender.tsnodejs/src/cdp/types.tsnodejs/src/cdp/utils.tsposthog/api/integration.pyposthog/cdp/test/test_validation.pyposthog/cdp/validation.pyposthog/models/integration/__init__.pyposthog/models/integration/email.pyposthog/settings/ses.pyproducts/integrations/mcp/tools.yamlproducts/workflows/backend/facade/api.pyproducts/workflows/backend/facade/contracts.pyproducts/workflows/backend/presentation/views/hog_flow.pyproducts/workflows/backend/presentation/views/hog_flow_template.pyproducts/workflows/backend/services/sandbox_sender.pyproducts/workflows/backend/templates/announce_a_new_feature_template.jsonproducts/workflows/backend/templates/celebrate-milestone.jsonproducts/workflows/backend/templates/feature-adoption-tips.jsonproducts/workflows/backend/templates/heavy-usage-detected.jsonproducts/workflows/backend/templates/high-intent-notification.jsonproducts/workflows/backend/templates/negative-survey-response.jsonproducts/workflows/backend/templates/onboarding_started_but_not_completed_template.jsonproducts/workflows/backend/templates/re-engagement-workflow.jsonproducts/workflows/backend/templates/repeated-failure-alert.jsonproducts/workflows/backend/templates/support-ticket-notification.jsonproducts/workflows/backend/templates/ticket-unresolved-alert.jsonproducts/workflows/backend/templates/trial-ending-reminder.jsonproducts/workflows/backend/templates/trial_started_upgrade_nudge_template.jsonproducts/workflows/backend/templates/unlocking_advanced_features.jsonproducts/workflows/backend/templates/unused-features-education.jsonproducts/workflows/backend/templates/user-stuck-onboarding.jsonproducts/workflows/backend/templates/user-upgrade-webhook.jsonproducts/workflows/backend/templates/welcome_email_sequence_template.jsonproducts/workflows/backend/test/test_sandbox_sender_lifecycle.pyproducts/workflows/backend/tests/api/test_email_reputation.pyproducts/workflows/backend/tests/api/test_hog_flow_template.pyproducts/workflows/backend/tests/api/test_sandbox_sender.pyproducts/workflows/backend/tests/api/test_sandbox_sender_validation.pyproducts/workflows/frontend/Broadcasts/broadcastTestSendLogic.tsproducts/workflows/frontend/Broadcasts/steps/BroadcastContentStep.tsxproducts/workflows/frontend/Channels/MessageChannels.stories.tsxproducts/workflows/frontend/Channels/MessageChannels.tsxproducts/workflows/frontend/Channels/SandboxEmailSenderRow.tsxproducts/workflows/frontend/TemplateLibrary/SendTestEmailModal.stories.tsxproducts/workflows/frontend/TemplateLibrary/SendTestEmailModal.test.tsxproducts/workflows/frontend/TemplateLibrary/SendTestEmailModal.tsxproducts/workflows/frontend/TemplateLibrary/messageTemplateTestSendLogic.test.tsproducts/workflows/frontend/TemplateLibrary/messageTemplateTestSendLogic.tsproducts/workflows/frontend/Workflows/WorkflowScene.tsxproducts/workflows/frontend/Workflows/hogflows/findTestSendSkipReason.test.tsproducts/workflows/frontend/Workflows/hogflows/findTestSendSkipReason.tsproducts/workflows/frontend/Workflows/hogflows/steps/components/HogFlowFunctionConfiguration.tsxproducts/workflows/frontend/Workflows/templates/WorkflowTemplateCard.tsxproducts/workflows/frontend/WorkflowsScene.tsxproducts/workflows/frontend/emptyState/workflowsEmptyState.tsxproducts/workflows/frontend/firstRun/FirstRunWorkflowBanner.stories.tsxproducts/workflows/frontend/firstRun/FirstRunWorkflowBanner.tsxproducts/workflows/frontend/firstRun/GalleryTemplateCard.tsxproducts/workflows/frontend/firstRun/MakeItYoursActions.tsxproducts/workflows/frontend/firstRun/MakeItYoursEmailList.tsxproducts/workflows/frontend/firstRun/TemplateStartsOnLine.tsxproducts/workflows/frontend/firstRun/WorkflowsFirstRun.stories.tsxproducts/workflows/frontend/firstRun/WorkflowsFirstRunGallery.tsxproducts/workflows/frontend/firstRun/WorkflowsFirstRunMakeItYours.stories.tsxproducts/workflows/frontend/firstRun/WorkflowsFirstRunMakeItYours.tsxproducts/workflows/frontend/firstRun/WorkflowsFirstRunTab.test.tsxproducts/workflows/frontend/firstRun/WorkflowsFirstRunTab.tsxproducts/workflows/frontend/firstRun/buildFirstRunTestSend.tsproducts/workflows/frontend/firstRun/buildFirstRunWorkflow.tsproducts/workflows/frontend/firstRun/firstRunGalleryLogic.test.tsproducts/workflows/frontend/firstRun/firstRunGalleryLogic.tsproducts/workflows/frontend/firstRun/firstRunMakeItYoursLogic.test.tsproducts/workflows/frontend/firstRun/firstRunMakeItYoursLogic.tsproducts/workflows/frontend/firstRun/firstRunSenderLogic.test.tsproducts/workflows/frontend/firstRun/firstRunSenderLogic.tsproducts/workflows/frontend/firstRun/firstRunStoryFixtures.tsproducts/workflows/frontend/firstRun/firstRunWorkflowBannerLogic.test.tsproducts/workflows/frontend/firstRun/firstRunWorkflowBannerLogic.tsproducts/workflows/frontend/firstRun/firstRunWorkflowStorage.tsproducts/workflows/frontend/firstRun/templateEmails.tsproducts/workflows/frontend/firstRun/withFirstRunSender.test.tsproducts/workflows/frontend/firstRun/withFirstRunSender.tsproducts/workflows/package.jsonservices/mcp/src/api/generated.ts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 6 remain after this review.
c90a96e to
6710258
Compare
|
Note 🤖 GPT-6.1 Sol responding on behalf of Michael Replaced by the draft fork PR #335 in its native fork stack. @meikelmosby will start migrating the stack from the fork into PostHog/posthog for upstream review and CI. Closing this master-targeted copy to keep the handoff clear. |
Stacked on #112053.
Problem
workflows-first-run.Note
🔵 Merge risk: low (merge-risk rubric)
workflows-first-runis on and the workflow is the first-run one; it only reads data and writes browser storage. Revert is clean.Changes
workflowLogic.WorkflowScene.tsx. Everything else is new files underproducts/workflows/frontend/firstRun/."View metrics" in the Sending story, clicked in a headless browser, opens the Metrics tab:
New surface, so there is no before.
How did you test this code?
Test seams: a kea logic test,
firstRunWorkflowBannerLogic.test.ts, mounts the realworkflowLogicwith the hog flow API mocked at the HTTP layer. Stories inFirstRunWorkflowBanner.stories.tsxcover both states at full width and at 520px.Test rationale: no existing test covers this banner. The logic test catches:
saveWorkflowPartial, as the header does);Evidence:
Red: the new test on a stub logic that never shows a banner
The dismissal test passed on the stub because it did not yet assert the draft banner first. It now does, and removing
persistfrom the dismissal reducer makes it fail.workflowSceneLogic.test.tspass (7 suites, 44 tests).typescript:checkclean,oxlint --quietandoxfmt --checkclean on the touched files, pre-pushci:preflight --strictclean.Release status
Automatic notifications
Docs update
None. The first run is behind a flag and has no docs yet.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Claude Opus 5.5 (
claude-opus-5-5)tdd,/writing-user-facing-copy,/writing-pr-descriptions. Repo conventions from/writing-kea-logics,/writing-testsand/writing-ui-componentswere followed from the existing first-run files.assets/fr-262-banners, because the PR asset uploader refused this account.🤖 Generated with Claude Code